Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide locations swap icon when fields are empty #895

Merged
merged 5 commits into from
Nov 2, 2024
Merged

Conversation

Altonss
Copy link
Collaborator

@Altonss Altonss commented Dec 6, 2023

Solves #732

@cla-bot cla-bot bot added the cla-signed ✔️ The Contributor Licence Agreement was signed by all contributors. label Dec 6, 2023
@Altonss Altonss self-assigned this Dec 6, 2023
@Altonss Altonss linked an issue Dec 6, 2023 that may be closed by this pull request
Copy link
Collaborator

@ialokim ialokim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

As discussed in #733 (comment) I'm still not convinced that removing the icon button completely is the right thing to do. Especially because there is another icon left of it that might jump around.

In any case I would postpone this fix until after the release.

@Altonss
Copy link
Collaborator Author

Altonss commented Jan 4, 2024

As discussed in #733 (comment) I'm still not convinced that removing the icon button completely is the right thing to do. Especially because there is another icon left of it that might jump around.

In any case I would postpone this fix until after the release.

Oh yes this would make sense, so preferably just disable this button right?

@ialokim
Copy link
Collaborator

ialokim commented Jan 7, 2024

As discussed in #733 (comment) I'm still not convinced that removing the icon button completely is the right thing to do. Especially because there is another icon left of it that might jump around.
In any case I would postpone this fix until after the release.

Oh yes this would make sense, so preferably just disable this button right?

Yes, but "just" disabling might not be that easy, in the last PR we didn't find a proper way of visually disabling an IconButton, iirc.

@Altonss
Copy link
Collaborator Author

Altonss commented Jan 7, 2024

Oh I see... Then isn't hiding a good compromise? I see for example that Trainline is also hiding their swap button https://www.thetrainline.com

@ialokim
Copy link
Collaborator

ialokim commented Jan 7, 2024

I'm mostly concerned about the favorite icon jumping around if this gets hidden. If swap is only ever hidden when favorite is hidden, too, I think I could live with it.

@Altonss
Copy link
Collaborator Author

Altonss commented Jan 7, 2024

I'm mostly concerned about the favorite icon jumping around if this gets hidden. If swap is only ever hidden when favorite is hidden, too, I think I could live with it.

I agree!

@Altonss
Copy link
Collaborator Author

Altonss commented Jan 7, 2024

Fixed it, now the icon isn't hidden anymore: it is disabled and greyed out. It works and looks great on my device :) I'd be happy to hear your feedback !

@Altonss Altonss requested review from ialokim and grote January 10, 2024 18:55
@Altonss
Copy link
Collaborator Author

Altonss commented Nov 1, 2024

As nobody seems currently arround to review the PR, I'll merge it as it is a minimal fix and suggestions from the previous review have been applied.
Feel free to comment/open an issue if there is anything about this change you wish to discuss :)

@Altonss Altonss merged commit a6ee797 into grote:master Nov 2, 2024
4 checks passed
@Altonss Altonss deleted the swap branch December 7, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed ✔️ The Contributor Licence Agreement was signed by all contributors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Swapping From and To fields sometimes results in an animation glitch
2 participants